Feature zms 3434 print info on ekiosk ticket #727
Conversation
WalkthroughThe pull request introduces modifications across several files in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
zmsticketprinter/templates/block/content/print.twig (1)
7-11: Consider improving text concatenation logicThe current implementation has two potential issues:
- Hardcoded period might cause double punctuation if printText already ends with punctuation
- Period concatenation might not be appropriate for all languages
Consider moving the period to the translation string itself for better localization support.
zmsticketprinter/src/Zmsticketprinter/Process.php (1)
59-59: Consider sanitizing printText before passing to templateWhile template escaping should be implemented, it's good practice to also sanitize data at the controller level.
Consider adding sanitization:
- 'printText' => $printText, + 'printText' => strip_tags($printText),zmsticketprinter/src/Zmsticketprinter/Index.php (1)
46-48: Consider using constant for translation keysTo avoid typos and maintain consistency, consider defining translation keys as class constants.
+ private const TRANSLATION_PRINT_TEXT = 'printText'; + $translations = [ - 'printText' => '' + self::TRANSLATION_PRINT_TEXT => '' ];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
zmsticketprinter/src/Zmsticketprinter/Index.php(2 hunks)zmsticketprinter/src/Zmsticketprinter/Process.php(2 hunks)zmsticketprinter/templates/block/content/buttons.twig(3 hunks)zmsticketprinter/templates/block/content/print.twig(1 hunks)
🔇 Additional comments (1)
zmsticketprinter/templates/block/content/buttons.twig (1)
Line range hint 17-74: Verify translation system integration
The new printText functionality relies heavily on the translation system. Let's verify its implementation and available translations.
✅ Verification successful
Let me gather more information about the translation system integration, particularly focusing on the language configuration and defaultPrintText setting.
Based on the gathered information, I can now provide a final response about the translation system integration:
Translation system is properly integrated with printText functionality
The implementation shows a well-structured translation system where:
- printText is correctly passed through hidden form fields in the templates
- The value comes from translations['printText'] which is properly initialized
- Language configuration is handled via base64 encoded JSON with fallback to default text
- Default German language handling is implemented with proper fallback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for translation files and printText key usage
echo "Searching for translation files..."
fd -e yml -e yaml . | while read -r file; do
echo "=== $file ==="
# Look for printText related translations
grep -A 2 "printText:" "$file" || true
done
# Search for other usages of printText in the codebase
echo -e "\nSearching for printText usage in PHP files..."
rg "printText" -t php
echo -e "\nSearching for printText usage in Twig templates..."
rg "printText" -t twig
Length of output: 9823
Script:
#!/bin/bash
# Search for language configuration and defaultPrintText settings
echo "Searching for language configuration files..."
fd -e php -e json -e yaml -e yml . | xargs rg -l "defaultPrintText|languageConfig" || true
echo -e "\nSearching for specific language configuration implementation..."
rg -A 5 "defaultPrintText" || true
echo -e "\nChecking Index.php implementation..."
cat zmsticketprinter/src/Zmsticketprinter/Index.php || true
Length of output: 6373
| {% if printText %} | ||
| . {{ printText }} | ||
| {% endif %} |
There was a problem hiding this comment.
Add HTML escaping for printText to prevent XSS
The printText value is rendered directly in the template without escaping, which could lead to XSS vulnerabilities if the content contains malicious HTML/JavaScript.
Apply this change:
- . {{ printText }}
+ . {{ printText|escape }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {% if printText %} | |
| . {{ printText }} | |
| {% endif %} | |
| {% if printText %} | |
| . {{ printText|escape }} | |
| {% endif %} |
| @@ -32,6 +32,7 @@ public function readResponse( | |||
| $validator = $request->getAttribute('validator'); | |||
| $scopeId = $validator->getParameter('scopeId')->isNumber()->getValue(); | |||
| $requestId = $validator->getParameter('requestId')->isNumber()->getValue(); | |||
| $printText = $validator->getParameter('printText')->isString()->getValue(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add length validation for printText parameter
While type validation is implemented, there's no maximum length restriction on the printText parameter. This could potentially lead to very large strings being processed and displayed.
Consider adding length validation:
- $printText = $validator->getParameter('printText')->isString()->getValue();
+ $printText = $validator->getParameter('printText')->isString()->matchLength(1, 255)->getValue();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $printText = $validator->getParameter('printText')->isString()->getValue(); | |
| $printText = $validator->getParameter('printText')->isString()->matchLength(1, 255)->getValue(); |
| if (empty($currentLang) || $currentLang === 'de') { | ||
| $translations['printText'] = $languageConfig['defaultPrintText'] ?? ''; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify language condition and validate config structure
The current implementation could be improved in two ways:
- The language condition could be simplified
- The languageConfig structure should be validated
Consider this approach:
+ if (!isset($languageConfig['defaultPrintText'])) {
+ throw new \InvalidArgumentException('Missing defaultPrintText in language config');
+ }
+
- if (empty($currentLang) || $currentLang === 'de') {
- $translations['printText'] = $languageConfig['defaultPrintText'] ?? '';
- }
+ $translations['printText'] = ($currentLang === 'de' || empty($currentLang))
+ ? $languageConfig['defaultPrintText']
+ : '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (empty($currentLang) || $currentLang === 'de') { | |
| $translations['printText'] = $languageConfig['defaultPrintText'] ?? ''; | |
| } | |
| if (!isset($languageConfig['defaultPrintText'])) { | |
| throw new \InvalidArgumentException('Missing defaultPrintText in language config'); | |
| } | |
| $translations['printText'] = ($currentLang === 'de' || empty($currentLang)) | |
| ? $languageConfig['defaultPrintText'] | |
| : ''; |
| @@ -14,6 +14,7 @@ | |||
| {{ hiddenfield({ "name": "scopeId", "value": ticketprinter.buttons|first.scope.id }) }} | |||
| {{ hiddenfield({ "name": "clusterId", "value": ticketprinter.buttons|first.cluster.id }) }} | |||
| {{ hiddenfield({ "name": "requestId", "value": button.requestId }) }} | |||
| {{ hiddenfield({ "name": "printText", "value": translations['printText'] }) }} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check and ensure XSS prevention
While adding the printText hidden field aligns with the PR objective, consider these security and robustness improvements:
- Add a null check for the translations array
- Ensure proper escaping of the translation value
-{{ hiddenfield({ "name": "printText", "value": translations['printText'] }) }}
+{{ hiddenfield({ "name": "printText", "value": translations['printText']|default('')|escape }) }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{ hiddenfield({ "name": "printText", "value": translations['printText'] }) }} | |
| {{ hiddenfield({ "name": "printText", "value": translations['printText']|default('')|escape }) }} |
| @@ -69,6 +71,7 @@ | |||
| {{ hiddenfield({ "name": "scopeId", "value": button.scope.id }) }} | |||
| {{ hiddenfield({ "name": "clusterId", "value": button.cluster.id }) }} | |||
| {{ hiddenfield({ "name": "requestId", "value": button.requestId }) }} | |||
| {{ hiddenfield({ "name": "printText", "value": translations['printText'] }) }} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent implementation with other blocks
Apply the same null check and escaping improvements as suggested for the other blocks.
-{{ hiddenfield({ "name": "printText", "value": translations['printText'] }) }}
+{{ hiddenfield({ "name": "printText", "value": translations['printText']|default('')|escape }) }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{ hiddenfield({ "name": "printText", "value": translations['printText'] }) }} | |
| {{ hiddenfield({ "name": "printText", "value": translations['printText']|default('')|escape }) }} |
| @@ -43,6 +44,7 @@ | |||
| {{ hiddenfield({ "name": "scopeId", "value": button.scope.id }) }} | |||
| {{ hiddenfield({ "name": "clusterId", "value": button.cluster.id }) }} | |||
| {{ hiddenfield({ "name": "requestId", "value": button.requestId }) }} | |||
| {{ hiddenfield({ "name": "printText", "value": translations['printText'] }) }} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent implementation with button_single
Apply the same null check and escaping improvements as suggested for the button_single block.
-{{ hiddenfield({ "name": "printText", "value": translations['printText'] }) }}
+{{ hiddenfield({ "name": "printText", "value": translations['printText']|default('')|escape }) }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{ hiddenfield({ "name": "printText", "value": translations['printText'] }) }} | |
| {{ hiddenfield({ "name": "printText", "value": translations['printText']|default('')|escape }) }} |
Description
Short description or comments
Reference
Issues #XXX
Summary by CodeRabbit
New Features
printTextin multiple button blocks to enhance form data submission.printTextin the print template to display additional information after the contact name.Bug Fixes
Documentation
readResponseto include a new variable for enhanced response rendering.